unsafe: Removes most of the unsafe casts.#58
Conversation
Rather than forcing casts from an *Object to the desired type, we store a "self" reference on the object to allow us to get back to the containing type. This does increase the amount of ram each object takes up (~2 more pointers), the chance of mis-casting code goes down dramatically. There is still a bad cast resulting from values coming out of WrapNative, and this has helped corner that cast. The actual fix is quite involved, but this should prevent similar problems in the future (hopefully).
|
Thanks for the PR! I definitely like the idea of making the code safer. I have a few concerns:
It'd be worth running the benchmarks to get a sense of some of these costs. |
|
Got a little bogged down at work. I think your concern about the performance is worth keeping an eye on. Unfortunately I made a bunch of aggressive changes in this which likely should be pulled apart and measured separately. Roughly I was seeing a 15% impact in performance on the python benchmark suite. The upside is I think I can recover some (more? unmeasured right now) by generating code that is easier for the go compiler to optimize. Regardless, in summary, I am going to split this up into smaller parts and get each part measured and we can look at them that way. |
|
@nairb774 have you some news on this? Or some pointers if anyone wants to continue your work? I migrated your issue to grumpyhome#81 (forked), where development is still ongoing. |
3 participants
Rather than forcing casts from an *Object to the desired type, we store a "self" reference on the object to allow us to get back to the containing type. This does increase the amount of ram each object takes up (~2 more pointers), the chance of miscasting code goes down dramatically.
There is still a bad cast resulting from values coming out of WrapNative, and this has helped corner that cast. The actual fix is quite involved, but this should prevent similar problems in the future (hopefully).
I expect that this might be a little controversial, but the WrapNative bug I am trying to pin down is the result of the following:
The returned value is a memory mashup of an Int and a Long. There is a similar test already, but it only uses a raw
uint64rather than something likefooabove.Thoughts?